Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebSocket NEXT: automatically close connection when OIDC extension provides SecurityIdentity and token expires #40857

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented May 27, 2024

This comment has been minimized.

This comment has been minimized.

Copy link

github-actions bot commented May 27, 2024

🙈 The PR is closed and the preview is expired.

@sberyozkin
Copy link
Member

Thanks @michalvavrik. Can users have a chance to react to this closure somehow, have the error handler called etc ?

@michalvavrik michalvavrik force-pushed the feature/websockets-close-onauth-expired branch from c52ca24 to 4de8315 Compare May 28, 2024 08:14
@michalvavrik
Copy link
Member Author

michalvavrik commented May 28, 2024

Thanks @michalvavrik. Can users have a chance to react to this closure somehow, have the error handler called etc ?

Connection is getting closed, you shouldn't be able to stop it. What else can be done?

If we only fired on error, there are 4 strategies (close, log, call, do nothing, custom OnError method) and only the first one is suitable. Please take this with big reservation because I am no expert but IMO: anything but closing is not thread-safe because we cannot ensure that the identity is updated before next on message is called and we don't have way to stop everything till re-authentication happens (like token refresh).

I have added assertion that OnClose is called, hope it helps.

This comment has been minimized.

This comment has been minimized.

@sberyozkin
Copy link
Member

sberyozkin commented May 28, 2024

@michalvavrik I'm thinking what the user experience will be like. Lets say we authenticate with Google into a Quarkus endpoint which returns HTML page, with an option to start a chatbot. The user clicks on it and a web socket session starts, I guess in this case we have the larger HTML page with text like Hello, Alice and then a smaller embedded WS window where a chat bot conversation is ongoing.

Lets say the session has expired. If now Alice presses on anything in the HTML page like a link to another HTML page, Quarkus may auto-refresh the session or redirect Alice to Google to re-authenticate.
But what if, after the session has expired, Alice types something in the WS channel, what will she see in the ChatBot window if the connection is closed ?

@mkouba
Copy link
Contributor

mkouba commented May 28, 2024

I have added assertion that OnClose is called, hope it helps.

+1

Lets say the session has expired. If now Alice presses on anything in the HTML page like a link to another HTML page, Quarkus may auto-refresh the session or redirect Alice to Google to re-authenticate.
But what if, after the session has expired, Alice types something in the WS channel, what will she see in the ChatBot window if the connection is closed ?

It's up to the client I would say. I.e. if the connection is closed then the client (e.g. some JS code) can decide to redirect the user.

@sberyozkin
Copy link
Member

Just curious, ideally, the user should realize that the ChatBot error related to the closed connection means it is time to refresh the surrounding HTML page

@michalvavrik michalvavrik force-pushed the feature/websockets-close-onauth-expired branch from 4de8315 to 514c426 Compare May 28, 2024 09:57
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loogs good!

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 28, 2024
@michalvavrik
Copy link
Member Author

@michalvavrik I'm thinking what the user experience will be like. Lets say we authenticate with Google into a Quarkus endpoint which returns HTML page, with an option to start a chatbot. The user clicks on it and a web socket session starts, I guess in this case we have the larger HTML page with text like Hello, Alice and then a smaller embedded WS window where a chat bot conversation is ongoing.

Lets say the session has expired. If now Alice presses on anything in the HTML page like a link to another HTML page, Quarkus may auto-refresh the session or redirect Alice to Google to re-authenticate. But what if, after the session has expired, Alice types something in the WS channel, what will she see in the ChatBot window if the connection is closed ?

I agree with Martin's comments - client has to handle this.

I have one idea, but it would need to be a separate issue and I don't know if it makes sense - for a code flow we know that token is going to expire and if refreshing token is possible asynchronously via WebClient POST request, we could do that when the expiration time is getting closer. However at one time right before we synchronize there would be old token (not yet expired) used by the identity and a new issued token.

Copy link

quarkus-bot bot commented May 28, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 514c426.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

You can consult the Develocity build scans.

@sberyozkin
Copy link
Member

It's up to the client I would say. I.e. if the connection is closed then the client (e.g. some JS code) can decide to redirect the user.

Sure, this is what I was curious about, as long as the script can intercept this error and decide what to do, it is all good

Copy link

quarkus-bot bot commented May 28, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 514c426.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@sberyozkin
Copy link
Member

Hi @michalvavrik

for a code flow we know that token is going to expire and if refreshing token is possible asynchronously via WebClient POST request, we could do that when the expiration time is getting closer. However at one time right before we synchronize there would be old token (not yet expired) used by the identity and a new issued token.

This is Ok I guess, the WS session will still use valid identity until it expires, even if outside of the WS channel a new token is available... Let's discuss it further when you'd like

@sberyozkin sberyozkin merged commit 22ec638 into quarkusio:main May 28, 2024
22 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 28, 2024
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone May 28, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label May 28, 2024
@sberyozkin
Copy link
Member

sberyozkin commented May 28, 2024

Proposing a backport to 3.11 as a hardening fix, which ensures the WS session can't go forever if the identity has expired. But, @gsmet, please remove the backport label if you have some concerns about backporting it

@michalvavrik michalvavrik deleted the feature/websockets-close-onauth-expired branch May 28, 2024 10:42
@gsmet gsmet modified the milestones: 3.12 - main, 3.11.1 Jun 4, 2024
@gsmet gsmet modified the milestones: 3.11.1, 3.12 - main Jun 4, 2024
@gsmet
Copy link
Member

gsmet commented Jun 4, 2024

I tried to backport that one but we would need to also backport #40655 for the tests to pass.

I will let @mkouba decide what's best: either backport both or none (or adjust the tests).

@mkouba
Copy link
Contributor

mkouba commented Jun 5, 2024

I tried to backport that one but we would need to also backport #40655 for the tests to pass.

I will let @mkouba decide what's best: either backport both or none (or adjust the tests).

I would backport both... if possible. Thanks.

@gsmet gsmet modified the milestones: 3.12 - main, 3.11.2 Jun 10, 2024
@gsmet
Copy link
Member

gsmet commented Jun 10, 2024

I would backport both... if possible. Thanks.

@mkouba done with 3.11.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

WebSockets Next: close the connection if the security identity has expired
4 participants